-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/#6 MVVM Refectoring #7
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ㅎㅇㅌㅎㅇㅌ
뭐가 잘안되는디?
struct SignInViewModel{ | ||
var name : Observable<String> | ||
let disposeBag = DisposeBag() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
struct로 한 이유가 있을까용?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
딱히 이유는 없어용!
굳이 찾자면... Struct는 스택메모리에 저장돼서 좀 가벼운 느낌이라..?
Struct 로 가능한 것 들은 최대한 Struct 로 하려고 하는데
찾아보니까 보통 ViewModel 구현할 때 Class 로 하는 것 같긴해요...ㅎ
이유는 좀 더 공부해보겠습니당
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wwdc16 understanding swift performance 나
struct 내 reference type이 많을 경우 어떤 오버헤드가 발생하는지 검색해보면 이유를 알수있을수도요 ㅎ
|
||
let _ = Observable | ||
.combineLatest(fetchEmail, fetchPassword) | ||
.subscribe(onNext: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
map -> bind 로 바꿀수 있을것같아요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드가 읽기 편하려면 최대한 일편적인 indenentation을 하려고 노력하면 좋습니당.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
핫 indenentation 피드백 너무 고맙슴당🥹
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
map -> bind 피드백 주셨는데 혹시 위 코드에서 map이 combineLatest 가리키는 건가용?
Observable | ||
.combineLatest(fetchPassword, fetchChechPassword) | ||
.map{ $0 == $1} | ||
.subscribe { checkCondition.onNext($0) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이거를 바로 isSamePW로 바인드해주는것 어떤가용
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵 한번 해보겠슴다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셧습니다... 알엑스 어려웠을 텐데 서린 최고.
목요일에 민재한테도 코드리뷰 받자 ㅎㅎ
override func viewDidLoad() { | ||
super.viewDidLoad() | ||
view.backgroundColor = .white | ||
|
||
setLoginCheckViewControllerLayout() | ||
confirmBtn.addTarget(self, action: #selector(didTapConfirmButton), for: .touchUpInside) | ||
} | ||
|
||
public func configEmail(_ email : String){ | ||
welcomeLabel.text = "\(email)님\n 환영합니다" | ||
setUI() | ||
setLayout() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
viewDidLoad()에 setDataBindRx 호출 안해줘서 안되는거아냐?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이거는 다른 뷰턴에서 호출해주는 함수여서! 좀이따가 조금 더 구체적으로 톡방에 올릴게!
@objc private func didTapConfirmButton() { | ||
self.dismiss(animated: true) | ||
delegate?.dismissNavigationController() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
대체적으로 화면전환에서 델리게이트를 사용했던데 사용한 이유가 무엇일까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
나 류트뷰컨 바꿔주는 방식으로 안 하고
모달 -> 네비게이션컨트롤러 -> Auth 뷰컨들
이렇게 했었어서! 담에는 루트뷰컨 바꾸는 방식으로 해볼게!
}else{ | ||
return 50 | ||
} | ||
indexPath.row == 0 ? 73 : 50 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
삼항연산자 사용 좋네유
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드 리뷰 받아서 고친~! ^~^
관련이슈 🍎🍏
흠..